Skip to content

Use Clang's logic for adding the default IR attributes to a function #31906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 21, 2020
Merged

Use Clang's logic for adding the default IR attributes to a function #31906

merged 1 commit into from
May 21, 2020

Conversation

rjmccall
Copy link
Contributor

A lot of attributes are essentially default target configuration, and we should only differ when there's a good reason to.

For the attributes we were already setting:

  • the ptrauth and target CPU/feature attributes are taken care of by Clang
  • I've updated the optsize/minsize attributes to the apparent intent
  • I've left the frame-pointer override in place for now

Fixes rdar://63289339, which was caused by Swift's ptrauth IR attributes getting out of sync with Clang's.

@rjmccall rjmccall requested review from jckarter and atrick May 20, 2020 08:31
@rjmccall
Copy link
Contributor Author

swiftlang/llvm-project#1245

@swift-ci Please test.

@rjmccall
Copy link
Contributor Author

@stephentyrone This is also setting the LLVM floating-point attributes to match Clang's by default, so that e.g. on Darwin x86-64 I see for a simple Swift function:

attributes #0 = {
  norecurse
  nounwind
  readnone
  "correctly-rounded-divide-sqrt-fp-math"="false"
  "frame-pointer"="all"
  "less-precise-fpmad"="false"
  "no-infs-fp-math"="false"
  "no-nans-fp-math"="false"
  "no-signed-zeros-fp-math"="false"
  "no-trapping-math"="false"
  "stack-protector-buffer-size"="8"
  "target-cpu"="penryn"
  "target-features"="+cx16,+cx8,+fxsr,+mmx,+sahf,+sse,+sse2,+sse3,+sse4.1,+ssse3,+x87"
  "unsafe-fp-math"="false
  "use-soft-float"="false"
}

I don't know if anything in here strikes you as problematic.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

jckarter
jckarter approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Thanks John!

@rjmccall
Copy link
Contributor Author

swiftlang/llvm-project#1245

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

@rjmccall
Copy link
Contributor Author

@compnerd Is there something special about the Windows builder that would make it not respect cross-repository testing?

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

@compnerd
Copy link
Member

Yeah, the cross-repository settings are not propagated to the checkout on Windows. @shahmishal was going to look into that (at least had expressed that he was interested in doing so a while ago).

@shahmishal
Copy link
Member

Yeah, the cross-repository settings are not propagated to the checkout on Windows. @shahmishal was going to look into that (at least had expressed that he was interested in doing so a while ago).

Yes, I don't if I will be able to for few weeks.

@rjmccall rjmccall force-pushed the clang-ir-attributes branch from 0bc8ec2 to 2f8ec9d Compare May 21, 2020 05:07
@rjmccall
Copy link
Contributor Author

swiftlang/llvm-project#1247

@swift-ci Please test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0bc8ec28b5df102a7846176dc865559c94820a29

@rjmccall
Use Clang's logic for adding the default IR attributes to a function.
4f54c75 
A lot of attributes are essentially default target configuration, and we should only differ when there's a good reason to.

For the attributes we were already setting:
- the ptrauth and target CPU/feature attributes are taken care of by Clang
- I've updated the optsize/minsize attributes to the apparent intent
- I've left the frame-pointer override in place for now

Fixes rdar://63289339, which was caused by Swift's ptrauth IR attributes getting out of sync with Clang's.
@rjmccall rjmccall force-pushed the clang-ir-attributes branch from 2f8ec9d to 4f54c75 Compare May 21, 2020 06:24
@rjmccall
Copy link
Contributor Author

swiftlang/llvm-project#1247

@swift-ci Please test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 2f8ec9d2c712671394e4e91435288be918caea9d

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 2f8ec9d2c712671394e4e91435288be918caea9d

@stephentyrone
Copy link
Contributor

I don't know if anything in here strikes you as problematic.

Those all seem appropriate to me.

@atrick atrick mentioned this pull request May 21, 2020
Merged
@rjmccall rjmccall merged commit 620b044 into swiftlang:master May 21, 2020
@rjmccall rjmccall deleted the clang-ir-attributes branch May 21, 2020 16:53
@rjmccall rjmccall mentioned this pull request May 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jckarter jckarter jckarter approved these changes

@atrick atrick Awaiting requested review from atrick

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rjmccall @swift-ci @compnerd @shahmishal @stephentyrone @jckarter